-
-
Notifications
You must be signed in to change notification settings - Fork 723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][IMP] stock_inventory_discrepancy: add configuration for enabling discrepancy check #1966
[16.0][IMP] stock_inventory_discrepancy: add configuration for enabling discrepancy check #1966
Conversation
# Get the system parameter configuration | ||
param = ( | ||
self.env["ir.config_parameter"] | ||
.sudo() | ||
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable") | ||
) | ||
if self.env.context.get("skip_exceeded_discrepancy", False) or param == "0": | ||
return super().action_apply_inventory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For early returns.
# Get the system parameter configuration | |
param = ( | |
self.env["ir.config_parameter"] | |
.sudo() | |
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable") | |
) | |
if self.env.context.get("skip_exceeded_discrepancy", False) or param == "0": | |
return super().action_apply_inventory() | |
if self.env.context.get("skip_exceeded_discrepancy", False): | |
return super().action_apply_inventory() | |
inventory_discrepancy_enable = ( | |
self.env["ir.config_parameter"] | |
.sudo() | |
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable") | |
) | |
if inventory_discrepancy_enable == "0": | |
return super().action_apply_inventory() |
1519072
to
0a89a0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review: LGTM
@OCA/stock-logistics-warehouse-maintainers |
0a89a0e
to
b36eb8a
Compare
This PR has the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review.
@AungKoKoLin1997 From a usability perspective I think that it would be much better to add a setting in res.config.settings to enable / disable it, and document it in the readme of the module. A checkbox in res.config.settings would then update the parameter that you have proposed. |
I agree |
@JordiBForgeFlow @LoisRForgeFlow The purpose of this update was more to pass tests from another module rather than providing additional config option to users, and as such I personally thought updating res.config.settings wouldn't be necessary (wasn't sure if there would be some users who'd like to disable the feature after installing the module with config setting when there are already other ways to bypass the check with adjusting location/warehouse settings). Having said that, if the community opts for having a field in res.config.settings, why not! |
Be careful. Adding code just for tests should trigger 'warnings'. But, that said, adding a configuration that is accessible for users is always a good process to enable/disable a feature. For several reasons:
|
Thank you. @AungKoKoLin1997 Can you please follow up to add a configuration? |
b0ba48c
to
78f5ade
Compare
78f5ade
to
726edca
Compare
@LoisRForgeFlow @JordiBForgeFlow I added the configuration from a usability perspective. Can you please review it? |
help="Enable to show line discrepancies in inventory and block validation " | ||
"if the discrepancy exceeds the threshold.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help="Enable to show line discrepancies in inventory and block validation " | |
"if the discrepancy exceeds the threshold.", | |
help="Block validation of the inventory adjustment if discrepancy exceeds the threshold.", |
Since the setting doesn't affect the UI.
<div class="o_setting_right_pane"> | ||
<label for="inventory_discrepancy_enable" /> | ||
<div class="text-muted"> | ||
Enable to show line discrepancies in inventory and block validation if the discrepancy exceeds the threshold. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
726edca
to
75165ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review. 👍
75165ab
to
b589a5c
Compare
b589a5c
to
d52a680
Compare
059f9e8
to
d85c615
Compare
@rousseldenis @LoisRForgeFlow @JordiBForgeFlow |
@kanda999 Could you please review? |
d85c615
to
477c445
Compare
@rousseldenis I changed the approach like we did for OCA/stock-logistics-workflow#1760. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review.
from . import res_config_settings | ||
from . import res_company |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest putting them in alphabetical order unless there is a reason not to.
477c445
to
6befc0b
Compare
/ocabot merge minor |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 0b8d62d. Thanks a lot for contributing to OCA. ❤️ |
This PR aims to resolve the compatibility issue between this module and other inventory adjustment related modules.
Related: #1711
@qrtl QT4850